Add native VNC panel with input routing and tests - #2878

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cmuxUITests/BrowserPaneNavigationKeybindUITests.swift`:
- Around line 1407-1427: The helper shouldSkipOnCI currently treats any
non-empty CI as true; update it so it only skips for GitHub-hosted heuristics
(GITHUB_ACTIONS == "true", NSHomeDirectory starts with "/Users/runner",
currentDirectoryPath contains "/runner/work/" or starts with "/Users/runner/")
and remove the generic CI fallback, or replace it with an explicit opt-in env
var (e.g., SKIP_UI_TESTS or CMUX_SKIP_UI_TESTS) that must be set to "1"/"true"
to skip; locate the function shouldSkipOnCI and either delete the guard that
checks environment["CI"] or change that branch to read and validate a namespaced
opt-in variable (trim, lowercased, accept only "1"/"true"/"yes") so
self-hosted/UTM VMs are not inadvertently skipped.

In `@cmuxUITests/MenuKeyEquivalentRoutingUITests.swift`:
- Around line 126-130: The test currently treats the precondition check
(reachedOnce from waitForGotoSplitMatch) as an XCTSkip which hides routing
regressions; replace the skip with a test failure so the replay assertion still
runs: change the throw XCTSkip(...) branch that checks reachedOnce into an
XCTFail("Cmd+E did not reach browser content on this CI runner") (do not
return/throw afterwards) so execution continues to the replay assertion and the
regression will surface; keep references to waitForGotoSplitMatch and
reachedOnce as the same condition.

In `@Sources/Panels/MarkdownPanel.swift`:
- Around line 1620-1629: The closure passed to
VncWebSocketProxyBridge.start(remoteHost:remotePort:...) can receive events from
stale bridge instances and overwrite the current proxyBridge; capture the local
bridge in the closure (e.g., let localBridge = bridge) and at the start of the
event handler guard that self?.proxyBridge === localBridge, returning early if
not, so only events from the currently-active bridge are processed; apply the
same capture-and-guard pattern for the other start(...) call handling at the
1821-1851 region to prevent stale-bridge events from affecting the replacement
bridge.
- Around line 1334-1343: InputTelemetry currently stores raw typed text in the
lastText property which exposes sensitive input via automation/state; remove the
lastText field (and any other storage of raw text at the other occurrences
referenced around lines 1473-1475 and 1935-1946) or change the implementation to
only record non-sensitive metadata (counts/timestamps) and gate any raw text
capture behind a strict fake-native UI-test flag (e.g., an explicit
isUITestFakeNative check) so production telemetry never contains plaintext user
input; update InputTelemetry and any places that write/read lastText to use the
safe metadata-only fields or the gated path.
- Around line 500-503: The NSError created with domain "cmux.vnc.proxy" and
userInfo [NSLocalizedDescriptionKey: "Invalid remote VNC port."] is using an
English literal; replace that raw string with a localized string using
String(localized: "vnc.invalidRemotePort", defaultValue: "Invalid remote VNC
port.") (or map to a stable error code enum that Swift maps to a localized
message) so lastErrorDetail receives a localized value; update the same pattern
where NSLocalizedDescriptionKey is set (the other similar throws near the other
VNC error sites) to use String(localized:..., defaultValue:...) or centralized
error-code→localized-string mapping for all occurrences.
- Around line 2026-2028: The code hotlinks noVNC's rfb.js and passes credentials
into its RFB constructor and internal methods (_requestRemoteResize,
_screenSize), which risks credential exfiltration and fragility; replace the
remote import with a vendored, pinned noVNC module shipped with the app and
change the script import to a relative local path (import RFB from
"./vendor/noVNC/core/rfb.js"), bundle/serve that file from the app assets, and
update all uses of RFB/sendCredentials/_requestRemoteResize/_screenSize to use
documented public APIs (or implement resize behavior in the surrounding code) so
you no longer patch underscored internals or call into third‑party code hosted
on another domain. Ensure the vendored copy is versioned/pinned and audited for
security before shipping.
- Around line 307-353: VncSSHHostAliasResolver.resolve(host:) runs a blocking
Process (ssh -G) and is called synchronously from `@MainActor` VncPanel, which can
freeze the UI; make resolution asynchronous and off-main: change the resolver to
an async/nonisolated API (or provide a completion handler) that launches the
Process on a background Task/DispatchQueue, avoid waitUntilExit by using
asynchronous reading of pipes and/or a timeout with explicit process.terminate()
to bound lifetime, and only hop back to the MainActor to apply the returned
candidate to VncPanel; keep logic that parses output and checks
process.terminationStatus but perform it off the main actor and ensure
errors/timeouts return nil.

In `@Sources/Panels/MarkdownPanelView.swift`:
- Around line 549-552: The current onChange handler calls panel.focus() whenever
panel.connectionState becomes .connected, which can steal keyboard focus; update
the handler (the onChange(of: panel.connectionState) block) to only call
panel.focus() when this panel is already the active/visible one (e.g., check a
focus/active flag such as panel.isFocused or panel.isActive) so that the call to
panel.focus() is gated (if state == .connected && panel.isFocused {
panel.focus() } or equivalent). Ensure you rely on the existing explicit focus
paths (onAppear / onChange(of: isFocused)) and do not change those.
- Around line 607-610: The host label inside the ForEach (iterating
panel.discoveredTargets) and Button currently uses a hard-coded string
interpolation "\(target.name) (\(target.endpoint))"; replace this with a
localized format using String(localized:..., defaultValue:...) so the
punctuation/ordering can be adapted for other locales and keep the variable
endpoint inserted via interpolation; update the Button title construction and
ensure the same localized format is used wherever the discovered-host label
appears and that the action still calls
panel.chooseEndpointSuggestion(target.endpoint).
- Around line 496-499: The overlay currently uses MarkdownPointerObserver which
only forwards left-button events and swallows other buttons; update this to use
a VNC-aware observer (e.g., VNCPointerObserver) or extend
MarkdownPointerObserver to forward the full mouse-button path so non-left clicks
reach the native VNC surface; specifically modify the overlay code that
instantiates MarkdownPointerObserver (and the observer implementation) to handle
all NSEvent/NSMouseButton types and pass them through or forward them to
onRequestPanelFocus (or the VNC input pipeline) instead of blocking them.
- Around line 701-715: connectOrDisconnect() currently bypasses the same
validation used by the UI button; before performing any action (connect,
submitCredentials, disconnect) check the existing isConnectButtonDisabled gate
and return early if it is true so keyboard Submit cannot trigger connect/submit
when fields are invalid. Specifically, at the top of connectOrDisconnect() call
the same boolean check (referencing isConnectButtonDisabled) and only proceed to
call panel.connect(), panel.submitCredentials(), panel.disconnect() when the
gate allows it; preserve the existing short-circuit logic for panel.isConnected
and panel.connectionState == .connecting but ensure the disabled-state check
prevents panel.connect() and panel.submitCredentials() from running.

In `@Sources/TabManager.swift`:
- Around line 5385-5401: The current code sets selectedTabId before attempting
to create the VNC surface, causing the UI to switch to tabId even if
newVncSurface(inPane:..., endpoint:..., autoConnect:..., focus:...,
insertAtEnd:...) returns nil; move the selectedTabId = tabId assignment so it
happens only after workspace.newVncSurface succeeds (i.e., after vncPanel is
non-nil), then call rememberFocusedSurface(tabId:tabId, surfaceId:vncPanel.id)
and return vncPanel.id, leaving tabs and workspace lookup logic unchanged.

In `@Sources/TerminalController.swift`:
- Around line 6184-6191: Remove the raw "input_last_text" value from the
socket/state payload generation: do not emit vncPanel.inputLastTextForAutomation
(the "input_last_text" key) in the payloads assembled in TerminalController;
instead emit non-sensitive metadata such as a presence flag or length (e.g.,
"input_last_text_present" or "input_last_text_length") or gate the raw string
behind a DEBUG-only test hook. Update the code paths that use
v2OrNull(vncPanel.inputLastTextForAutomation) to return the safe metadata (or
omit the key entirely) and apply the same change to the other affected block
referenced in the review (lines around 12700-12728); keep references to
vncPanel, v2OrNull, and the "input_last_text" key so you can find and modify the
two payload locations.
- Around line 6141-6173: Resolve and validate the incoming surface_id before
selecting a TabManager/Workspace: call v2UUID(params, "surface_id") up front and
return .err(code: "invalid_params", ...) if it's missing/invalid instead of
falling back to ws.focusedPanelId; then check ownership by seeing if the
resolved surfaceId is in the workspace's panels (ws.panels[surfaceId]) and if
not use AppDelegate.shared?.locateSurface(surfaceId:) to find the correct
(tabManager, workspace) pair and only then bind to that workspace; update uses
of v2ResolveTabManager(params:) and v2ResolveWorkspace(params:tabManager:) so
you only pick a tabManager when it actually owns the surface, and ensure error
responses use "invalid_params" for bad incoming ids and "not_found" when
locateSurface fails before dereferencing vncPanel.

In `@Sources/Workspace.swift`:
- Around line 9274-9280: The VNC panel seeding bypasses the normalization done
in VncPanel.init(workspaceId:endpoint:) and restoreSessionSnapshot(_:): trim the
endpoint and username before assigning to VncPanel (e.g., when creating
VncPanel(workspaceId:endpoint:) and then setting vncPanel.usernameInput and
vncPanel.passwordInput), so call .trimmingCharacters(in:
.whitespacesAndNewlines) on endpoint and username prior to assignment while
leaving passwordInput unchanged; update both identical call sites that set
vncPanel.usernameInput/passwordInput accordingly.

In `@vendor/bonsplit`:
- Line 1: The parent repo's submodule pointer for vendor/bonsplit was updated to
commit 2979ef647d4a6ab0bf7f93ce82c59cbf0c70e1e1 which does not exist on
vendor/bonsplit's origin/main (current submodule HEAD is
5d508dc0773a1cbc62390ca8446cfeaebd8a7d52), so revert the pointer change in the
parent commit until that commit is pushed to the vendor/bonsplit remote; either
push the missing commit to vendor/bonsplit's main branch or reset the submodule
pointer in the parent to an existing commit (e.g.,
5d508dc0773a1cbc62390ca8446cfeaebd8a7d52) and update the submodule cleanly
(using git submodule update --init --recursive) before reopening the PR.

---

Outside diff comments:
In `@cmuxUITests/MenuKeyEquivalentRoutingUITests.swift`:
- Around line 325-344: The helper refocusWebView currently swallows the final
focus checks; change its signature to return a Bool indicating whether the
WKWebView regained first responder and update its internals to return the result
of the final wait predicate (use waitForGotoSplitMatch with the predicate
data["webViewFocusedAfterAddressBarExit"] == "true" rather than discarding it),
and ensure the intermediate escape retry still occurs but its outcome feeds into
the returned Bool; then update every call site to assert the returned Bool (fail
the test if false) so tests require the web view to actually regain focus.

In `@Sources/AppDelegate.swift`:
- Around line 8563-8592: The test can activate a different "fallback" window
because mainTerminalWindow() searches NSApp.windows; change
runSetupWhenWindowReady() to resolve the UI window from the same TabManager
you're about to mutate: use a TabManager-backed window reference (e.g., a
property or method on TabManager such as tabManager.mainWindow or
tabManager.windowForUITests) instead of calling mainTerminalWindow(), and when
making the window key/ordering or calling orderFrontRegardless(), operate on
that TabManager-derived window; you can also update mainTerminalWindow() to
accept a TabManager parameter or add a new helper that prefers tabManager's
window to avoid the XCTest fallback race.

In `@Sources/TerminalController.swift`:
- Around line 14416-14445: The manual tokenization using args.split(separator: "
") in the TerminalController parsing block breaks quoted credentials (e.g.
--password="two words"); replace the space-splitting logic and per-part handling
with the same option parser used by vncState(_:) so flags with spaces/quotes are
parsed correctly. Locate the parsing block that iterates over parts (the code
using args.split and symbols like parseSplitDirection, panelType, direction,
url, endpoint, username, password, autoConnect) and route the raw args string
into the shared option-parsing routine (the one vncState(_) uses) or reuse the
parsing helper used by newVncSplit/newVncSurface, ensuring quoted values are
preserved and then map the resulting parsed options to
panelType/direction/url/endpoint/username/password/autoConnect. Ensure both
occurrences (this block and the similar block around newVncSplit/newVncSurface)
are updated to use the same parser.

---

Nitpick comments:
In `@cmuxUITests/BrowserPaneNavigationKeybindUITests.swift`:
- Around line 1379-1390: The helper ensureForegroundAfterLaunch currently waits
the full timeout before trying to recover from a .runningBackground launch;
change its logic so you check app.state early and do not burn the full timeout:
first, if app.state == .runningBackground then call app.activate() and
immediately wait a short recovery timeout (e.g., 1–2s) for .runningForeground;
otherwise try a short initial wait for .runningForeground and only if that fails
and state is .runningBackground call activate() and wait a slightly longer
recovery window; update the function ensureForegroundAfterLaunch to use these
shorter/fallback waits to avoid waiting the entire timeout before calling
activate().
- Around line 1267-1280: The clickBrowserPane helper falls back to a global
BrowserWebViewSurface.firstMatch which ignores the requested panelId and makes
clicks nondeterministic; update the fallback to scope the webview search to the
requested panelId (e.g., find the BrowserPanelContent.\(panelId) element and
then look for its descendant with accessibility identifier
"BrowserWebViewSurface" or use a panel-scoped identifier like
"BrowserWebViewSurface.\(panelId)"), use that scoped element for the click and
for the XCTAssertTrue check so the fallback remains panel-specific and
deterministic.

In `@Sources/AppDelegate.swift`:
- Around line 8594-8612: Extract the repeated four
KeyboardShortcutSettings.setShortcut calls (the StoredShortcut(...) entries for
.focusLeft, .focusRight, .focusUp, .focusDown) into a single helper function
(e.g., seedGotoSplitShortcuts or applyGotoSplitShortcuts) and call that helper
both here (inside the if !useGhosttyConfig block) and at the other setup
location where they are currently duplicated; the helper should encapsulate
creation of the StoredShortcut instances and the four setShortcut calls so
future changes stay in one place.

In `@Sources/Workspace.swift`:
- Around line 451-452: Add a unit test that exercises the VNC session snapshot
and rehydrate path by creating a VNC session, calling sessionSnapshot(), then
calling restoreSessionSnapshot() and asserting the restored
SessionVncPanelSnapshot fields (endpoint, username, customTitle, autoConnect)
match the original; update test coverage to include the vncSnapshot property
alongside markdownSnapshot and ensure the restore path uses the
SessionVncPanelSnapshot deserialization (check functions/session paths that
reference vncSnapshot and restoreSessionSnapshot); repeat the same round-trip
test pattern for other similar spots referenced (around the blocks at the other
occurrences) so endpoint/username/title/auto-connect are validated end-to-end.

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="cmuxUITests/BrowserPaneNavigationKeybindUITests.swift">

<violation number="1" location="cmuxUITests/BrowserPaneNavigationKeybindUITests.swift:1380">
P2: `ensureForegroundAfterLaunch` waits for foreground before attempting activation, so background launches can incur an unnecessary full timeout per test.</violation>
</file>

<file name="Sources/Panels/MarkdownPanelView.swift">

<violation number="1" location="Sources/Panels/MarkdownPanelView.swift:444">
P2: Do not disable the connect button during `.connecting`; the action handler already uses that state to cancel/disconnect, and disabling the button makes that recovery path unreachable.</violation>

<violation number="2" location="Sources/Panels/MarkdownPanelView.swift:551">
P2: Only auto-focus the VNC view on connect when this panel is focused. The current code can steal first responder from another active pane when a background VNC connection finishes.</violation>
</file>

<file name="cmuxUITests/MenuKeyEquivalentRoutingUITests.swift">

<violation number="1" location="cmuxUITests/MenuKeyEquivalentRoutingUITests.swift:330">
P2: Assert that WebKit focus restoration succeeded; otherwise these shortcut tests can pass without exercising the intended first-responder state.</violation>
</file>

<file name="Sources/Panels/MarkdownPanel.swift">

<violation number="1" location="Sources/Panels/MarkdownPanel.swift:1610">
P2: Resolve SSH host aliases off the main actor. Running `ssh -G` synchronously in `connect()` can freeze the UI while connecting.</violation>

<violation number="2" location="Sources/Panels/MarkdownPanel.swift:2027">
P1: Avoid loading the core VNC script from a remote URL. Vendoring/pinning noVNC locally is needed to prevent supply-chain exposure and runtime breakage.</violation>
</file>

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@Sources/Panels/MarkdownPanel.swift`:
- Around line 311-313: VncSSHHostAliasResolver.resolveBlocking currently calls
Process.waitUntilExit() with no timeout, so update resolveBlocking (and any
similar call sites used by VncSSHHostAliasResolver.resolve) to enforce a bounded
lifetime: start the ssh -G Process, schedule a timeout (e.g., DispatchQueue or
Task.sleep) and if the process hasn’t exited by deadline call
process.terminate(), then if still running forcefully kill and treat as failure;
ensure you capture the termination status/error and return/fail promptly to the
awaiting caller (resolve) instead of blocking forever; reference functions:
VncSSHHostAliasResolver, resolveBlocking, resolve, and usages of waitUntilExit()
and terminationHandler when locating the code to change.
- Around line 1361-1370: The InputTelemetry struct currently captures plaintext
user input via the lastText property and that plaintext is later exposed in
automation state; remove this sensitive data capture by deleting the lastText
property from InputTelemetry, remove any assignments that set lastText (e.g., in
input handling code that updates InputTelemetry.lastText), and remove any code
that exposes the plaintext through automation/state APIs (e.g., the automation
state accessor or serialization that returns lastText). If you still need
telemetry about typing, replace plaintext with non-sensitive alternatives such
as counts (textInputCount), a boolean flag indicating recent typing, or an
integer lastTextLength or a hashed/entropy-only metric; update the code paths
that previously consumed lastText to use the new non-sensitive field instead
(e.g., where automation state previously read lastText).

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Sources/Panels/MarkdownPanelView.swift`:
- Around line 502-514: The connect button can be disabled while a connection is
in progress because isConnectButtonDisabled falls through to endpoint
validation; add an early check for the connecting/cancel path (e.g., if
panel.isConnecting { return false }) at the top of isConnectButtonDisabled so
the UI keeps the visible cancel action enabled (remember connectOrDisconnect()
treats .connecting as the cancel/disconnect case).
- Around line 795-810: The focusMostUsefulField() currently always sets
focusedField = .password after checking username, which ignores
panel.requiredCredentialFields; instead, when panel.isAwaitingCredentials is
true, inspect panel.requiredCredentialFields in order and set focusedField to
the first required credential whose corresponding input (e.g.,
panel.usernameInput, panel.passwordInput) is empty; if none are empty, leave
focus or default to .endpoint. Update focusMostUsefulField() to use
panel.requiredCredentialFields and the inputs to pick the next missing required
field rather than unconditionally assigning .password.

---

Nitpick comments:
In `@Sources/Panels/MarkdownPanelView.swift`:
- Around line 472-859: The VncPanelView and its two NSViewRepresentable helpers
(VncPanelNativeSessionRepresentable, VncPanelWebViewRepresentable) should be
extracted from the large MarkdownPanelView.swift into a new source file so the
VNC UI is isolated: create a new Swift file containing struct VncPanelView, the
private enum Field, and the two representables, keep the same types/signatures
(VncPanelView, VncPanelNativeSessionRepresentable, VncPanelWebViewRepresentable)
and preserve all referenced state/props (panel, isFocused, isVisibleInUI,
portalPriority, onRequestPanelFocus) and methods (connectOrDisconnect,
focusMostUsefulField, triggerFocusFlashAnimation, focusFlashAnimation) but
import or make accessible any shared symbols used by the VNC code
(FocusFlashPattern, cmuxAccentColor(), MarkdownPointerObserver, CmuxWebView,
etc.)—adjust access control if needed and remove the moved definitions from the
original file, then build to fix any missing imports or visibility issues.

In `@Sources/Workspace.swift`:
- Around line 9251-9330: newVncSplit(...) and newVncSurface(...) both duplicate
VncPanel seeding logic (trimming endpoint/username, assigning
usernameInput/passwordInput, storing in panels/panelTitles/surfaceIdToPanelId,
calling installVncPanelSubscription and handling autoConnect); extract this into
a single helper like
seedVncPanel(endpoint:username:password:autoConnect:surfaceId:) that returns the
created VncPanel and performs: trim inputs, init VncPanel(workspaceId: id,
endpoint:), set usernameInput/passwordInput if present, insert into
panels[vncPanel.id], panelTitles[vncPanel.id], surfaceIdToPanelId[newTab.id]
(accept surfaceId param), call installVncPanelSubscription(vncPanel) and
schedule autoConnect via DispatchQueue.main.async, then update newVncSplit and
newVncSurface to call that helper and remove the duplicated code.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Sources/Panels/MarkdownPanel.swift`:
- Around line 1594-1599: The snapshot is using runtime connection state
(isConnected || connectionState == .connecting) instead of the configured
auto-connect flag; change sessionSnapshot() to read and persist a stored
autoConnect property on VncPanel (e.g., add/use a Bool property named
autoConnect on VncPanel) and pass that value into
SessionVncPanelSnapshot(autoConnect:), rather than computing it from isConnected
or connectionState; ensure anywhere the panel configuration is created/updated
(constructors or UI bindings) sets VncPanel.autoConnect so the snapshot reflects
the user-configured setting.
- Around line 1652-1658: The async host-resolution check only guards the
resolution path but not later native session callbacks, so stale
VncNativeSessionController `.connected`/`.disconnected` handlers can overwrite
state; update all places that register or handle native session callbacks (e.g.,
where VncNativeSessionController invokes connection state handlers and in
startNativeConnection and any disconnectFromCurrentSession(suppressUpdate:)
callback paths) to capture the current connectAttemptID and early-return if the
stored connectAttemptID != captured attemptID, effectively gating callback work
to the same connection generation (apply the same guard to the other occurrences
noted around the 1866–1868 and 1919–1943 regions).
- Around line 1099-1110: The code currently trims username and password before
encoding (usernameTrimmed/passwordTrimmed) which strips significant
leading/trailing whitespace and breaks valid VNC credentials; remove the
trimming and use the original username and password values when building
authComponent (and when calling Self.encodeUserInfo) so whitespace is preserved,
and likewise update hasCredentialValue(...) to stop trimming input there; also
apply the same change in the other occurrence referenced (around the
hasCredentialValue block at the 1815–1820 area) so whitespace-only or
space-padded credentials can be submitted and authenticated.
- Around line 228-230: The code accepts endpoints like "host:" or "host:abc" by
treating a malformed explicit port as “no port supplied” and defaulting to 5900;
fix this by explicitly validating the portString when
normalizedRemainder.hasPrefix(":") — ensure portString is non-empty and
Int(portString) succeeds before calling resolvedPort, and if parsing fails
return nil (do not fall through to default port). Update the three locations
that use normalizedRemainder, portString and resolvedPort (the occurrences
around the shown snippet plus the blocks at the other reported ranges) to first
check portString.isEmpty, then let portInt = Int(portString), then let port =
resolvedPort(portInt) else return nil.
- Around line 2054-2057: The JS error "line ..." prefix is not localized; update
the two components.append calls in MarkdownPanel (the branch that appends "line
\(line):\(column)" and the one that appends "line \(line)") to use
String(localized:..., defaultValue:...) like the other VNC error strings so the
UI shows localized builds correctly; pick clear localization keys (e.g. a key
for "line {line}:{column}" and one for "line {line}") and interpolate the
line/column values into the localized strings when calling String(localized:...,
defaultValue:...).

---

Nitpick comments:
In `@cmuxUITests/VncPanelInputRoutingUITests.swift`:
- Around line 16-64: The test
testVncPanelRoutesKeyboardMultiKeyAndMouseInputInNativeSession only exercises
left click/drag; add an explicit right-click and an other-button click using the
same content coordinates (e.g., perform a right click on center and an
otherMouse click on another coordinate via the content.coordinate(...) helpers)
before the final waitForVncState, and then extend the state-check closure to
assert the new counters (e.g., check
input_right_mouse_down_count/input_right_mouse_up_count and
input_other_mouse_down_count/input_other_mouse_up_count are >= 1) alongside the
existing mouse/key counters so the test fails if right/other button routing
regresses.